Skip to content

fix(paths): gracefully handle nulls for local cleaning filters#26085

Merged
thmsobrmlr merged 3 commits intomasterfrom
fix-paths-null-for-local-cleaning-filter
Nov 11, 2024
Merged

fix(paths): gracefully handle nulls for local cleaning filters#26085
thmsobrmlr merged 3 commits intomasterfrom
fix-paths-null-for-local-cleaning-filter

Conversation

@thmsobrmlr
Copy link
Collaborator

@thmsobrmlr thmsobrmlr commented Nov 8, 2024

Problem

Duplicating an insight leads to nulls in the query. For the local path cleaning filters this resulted in a crash for a customer.

Changes

Gracefully handles null values for the local path cleaning filter.

Follow-ups

  • Look into wether duplicating an insight creates these nulls, and if so - fix that -> could not reproduce
  • Python doesn't distinguish between null and undefined, meaning we have no protection from "faulty" queries like this in our backend side validation. Likely we should add nulls to the whole schema.ts types where we have an optional value.

How did you test this code?

Used the query from the customer locally to verify the issue and fix.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2024

Size Change: 0 B

Total Size: 1.15 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.15 MB

compressed-size-action

@thmsobrmlr thmsobrmlr requested a review from a team November 8, 2024 11:13
Copy link
Contributor

@anirudhpillai anirudhpillai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Regarding Python doesn't distinguish between null and undefined what's the exact case we need to distinguish between null and undefined on the backend? (just checking if the field was ever set?)

@thmsobrmlr
Copy link
Collaborator Author

thmsobrmlr commented Nov 11, 2024

Regarding Python doesn't distinguish between null and undefined what's the exact case we need to distinguish between null and undefined on the backend? (just checking if the field was ever set?)

So the thing is that we validate the queries backend side, where null vs undefined doesn't make a difference. On the frontend however the code could depend on wether something is undefined or null, as it does in this PR. At the moment we have our types set up assuming that the backend can only return undefined, but not null for most properties.

Meaning we can't change the validation on the backend, but we can change the definitions in schema.ts.

@thmsobrmlr thmsobrmlr merged commit 9bd920a into master Nov 11, 2024
@thmsobrmlr thmsobrmlr deleted the fix-paths-null-for-local-cleaning-filter branch November 11, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants